Value Types: Replace arraycopy source and destination with temps #7582
Conversation
When preparing for null restricted arraycopy transformation, there are temps created for the source array and the destination array. Other sub transformations that run after the preparation and before null-restrcited arraycopy transformation are not aware of these temps. These temps might not get updated properly. This change replaces all the commoned source array node and commoned destination array node with the temps right after the temps are created. Fixes: eclipse-openj9/openj9#20522 Signed-off-by: Annabelle Huo <[email protected]>
a7ehuo
changed the title
|
@hzongaro May I ask you to review this change? Thank you! |
| @@ -922,6 +922,47 @@ bool OMR::ValuePropagation::transformUnsafeCopyMemoryCall(TR::Node *arrayCopyNod | |||
| return false; | |||
| } | |||
|
|
|||
| static void setNodeOnList(TR::Node *currNode, TR_BitVector *nodeList) | |||
There was a problem hiding this comment.
May I ask you to add some Doxygen comments or at least some description of what it's doing? I know there's a bit of a description that appears where it's called, but I think it would help to have it described here.
| } | ||
| } | ||
|
|
||
| static void findAndReplaceCommonedNodeWithLoadFromTemp(TR::Node *currNode, TR::Node *matchingNode, TR::SymbolReference *symRef, TR_BitVector *nodeSkipList, bool trace, TR::Compilation *comp) |
There was a problem hiding this comment.
May I ask you to add some Doxygen comments or at least some description of what it's doing? I know there's a bit of a description that appears where it's called, but I think it would help to have it described here.
| if (trace) | ||
| traceMsg(comp, "%s: currNode n%dn replace childNode n%dn with loadNode n%dn\n", __FUNCTION__, currNode->getGlobalIndex(), childNode->getGlobalIndex(), loadNode->getGlobalIndex()); | ||
| } | ||
| else if (!nodeSkipList->isSet(childNode->getGlobalIndex())) |
There was a problem hiding this comment.
I find it a little bit confusing that a node that is in nodeSkipList might get processed by the if that appears just before this else if if its parent was not in nodeSkipList. I wonder if it should be called skipDescendantsList or something like that. Just a thought.
| findAndReplaceCommonedNodeWithLoadFromTemp(itNode, dstObjNode, dstArrRefSymRef, nodeSkipList, trace(), comp()); | ||
| findAndReplaceCommonedNodeWithLoadFromTemp(itNode, srcObjNode, srcArrRefSymRef, nodeSkipList, trace(), comp()); |
There was a problem hiding this comment.
I understand that the problem this is fixing appeared with temporaries for the destination or source arrays, but is it conceivable that the same problem could occur with the offset or length values?
There was a problem hiding this comment.
Good question! I think theoretically speaking the same problem could occur with the offset and length as well. I tried to tweak my unit tests to show this problem on the offset or length, but so far I haven't been able to get the expected IL trees. I'll think over the proper fix.
There was a problem hiding this comment.
I'm able to reproduce the same problem with copy size now. Below example of IL trees shows the copy size is stored to a temp #474 after the preparation of null restricted array copy transformation. If the slow path block_23 is invoked, #474 is uninitialized.
I will expand this change to all children of arraycopy in the next commit.
...
n180n istore <temp slot 12>[#465 Auto] [flags 0x3 0x0 ] [0x7f63f9f82800] bci=[-1,37,129] rc=0 vc=0 vn=- li=- udi=- nc=1
n32n iload <auto slot 3>[#421 Auto] [flags 0x3 0x0 ] (cannotOverflow ) [0x7f63f9ee5120] bci=[-1,36,129] rc=2 vc=51 vn=- li=- udi=- nc=0 flg=0x1000
n368n istore <temp slot 24>[#477 Auto] [flags 0x3 0x0 ] [0x7f63f9f862c0] bci=[-1,36,129] rc=0 vc=0 vn=- li=- udi=- nc=1
n32n ==>iload
n365n ificmpne --> block_23 BBStart at n168n () [0x7f63f9f861d0] bci=[-1,0,125] rc=0 vc=0 vn=- li=- udi=- nc=2 flg=0x20
n363n iand [0x7f63f9f86130] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=2
n361n iloadi <isClassFlags>[#340 Shadow +36] [flags 0x603 0x0 ] [0x7f63f9f86090] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=1
n360n aloadi <vft-symbol>[#341 Shadow] [flags 0x18607 0x0 ] [0x7f63f9f86040] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=1
n357n aload <temp slot 10>[#463 Auto] [flags 0x7 0x0 ] [0x7f63f9f85f50] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=0
n362n iconst 0x2000000 (X!=0 X>=0 ) [0x7f63f9f860e0] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x104
n364n iconst 0 (X==0 X>=0 X<=0 ) [0x7f63f9f86180] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x302
n405n BBEnd </block_14> ===== [0x7f63f9f86e50] bci=[-1,0,125] rc=0 vc=0 vn=- li=- udi=- nc=0
...
n366n BBStart <block_24> (freq 9500) [0x7f63f9f86220] bci=[-1,36,129] rc=0 vc=0 vn=- li=- udi=- nc=0
n292n istore <temp slot 21>[#474 Auto] [flags 0x3 0x0 ] [0x7f63f9f84b00] bci=[-1,36,129] rc=0 vc=0 vn=- li=- udi=- nc=1
n372n iload <temp slot 24>[#477 Auto] [flags 0x3 0x0 ] [0x7f63f9f86400] bci=[-1,36,129] rc=1 vc=0 vn=- li=- udi=- nc=0
...
n289n BBStart <block_17> (freq 9500) [0x7f63f9f84a10] bci=[-1,37,129] rc=0 vc=0 vn=- li=- udi=- nc=0
n36n ireturn [0x7f63f9ee5260] bci=[-1,41,130] rc=0 vc=51 vn=- li=- udi=- nc=1
n298n iload <temp slot 21>[#474 Auto] [flags 0x3 0x0 ] [0x7f63f9f84ce0] bci=[-1,36,129] rc=1 vc=0 vn=- li=- udi=- nc=0
n2n BBEnd </block_17> ===== [0x7f63f9ee47c0] bci=[-1,41,130] rc=0 vc=50 vn=- li=- udi=- nc=0
When preparing for null-restricted arraycopy transformation, there are temps created for the source array and the destination array. Other sub transformations that run after the preparation and before null-restricted arraycopy transformation are not aware of these temps. These temps might not get updated properly. This change replaces all the commoned source array node and commoned destination array node with the temps right after the temps are created.
Fixes: eclipse-openj9/openj9#20522